Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add vertico-crm-mode in separate package #74

Closed
wants to merge 2 commits into from
Closed

Add vertico-crm-mode in separate package #74

wants to merge 2 commits into from

Conversation

minad
Copy link
Owner

@minad minad commented Jul 6, 2021

Now I am experimenting with an enhanced vertico crm UI. This is a much more user friendly variant of consult-completing-read-multiple. Just turn on vertico-crm-mode and be done.

I think it is also more maintainable than consult-completing-read-multiple, which will be a hot mess if the implementation is scattered over Vertico, Embark and Consult. Downside is that we lose the nice portability aspect of consult-completing-read-multiple. These are the advantages:

  • No flicker, no restart of the minibuffer, no completing-read in a loop
  • No double-RET
  • Robust sorting
  • Filter string can be kept during selection (press tab vs backtab)
  • Embark works out of the box with this and we can offer more advanced keybindings.

By keeping the implementation strictly separate from vertico.el, I am not complicating Vertico. This was something I had been afraid of before. In my initial experiments I implemented crm in a much more intrusive fashion directly in vertico.el, which wasn't such a good idea and led me to explore the totally separate consult-completing-read-multiple implementation.

@minad
Copy link
Owner Author

minad commented Jul 6, 2021

@bdarcus A lot of back and forth here - sorry about that. But now I have at least explored this from all sides. Please give it a test.

@bdarcus
Copy link

bdarcus commented Jul 6, 2021

A lot of back and forth here - sorry about that.

No problem; one of the consequences of these decoupled packages!

I think you're fixing a few bugs as I'm testing, but here's what I see so far:

  1. big 👍 on the improvements; not that the lack of these were a big problem, but this is definitely more polished
  2. one UI problem I don't remember from earlier versions (?) is the "selected" group scrolls off screen if I move down; ideally the selected group should stick somehow?
  3. shift-TAB: I was going to ask if it would make sense to have a defcustom to toggle the TAB/shift-TAB behavior (preserve filter string vs remove), but now realize there may be a bug here. Sometimes shift-TAB works for me, and sometimes not. Now it doesn't. You may have fixed this though.
  4. initial-input either doesn't work or is buggy; I assume you know this, see screenshot below.
  5. I don't think embark is working correctly. It only seems to act on one candidate ATM.

Screenshot from 2021-07-06 11-55-37

@minad
Copy link
Owner Author

minad commented Jul 6, 2021

  1. No, I don't want to introduce sticky groups. Earlier versions also scrolled over the groups. The "Selected" group is a normal group and it is also filtered. There is no concept of a sticky group in Vertico itself. But you can always jump to the top to see all selected items.
  2. TAB keeps the input, S-TAB resets. I swapped the keybinding in contrast to what you originally proposed. It works for me.
  3. consult-completing-read-multiple behaved the same way. I am arguing that you abuse the initial-input for filtering. We discussed this before. It is unclear how a general solution for initial-input would look like, see my New multi-selection UI emacs-citar/citar#163 (comment).
  4. Yes, Embark acts only on the current candidate as expected. Injection works (this didn't work out of the box with consult-completing-read-multiple and required an embark-setup-hook). Adding a multi candidate target finder to Embark is an orthogonal issue. As soon as Vertico CRM becomes available we can discuss this on the Embark issue tracker.

@minad
Copy link
Owner Author

minad commented Jul 6, 2021

@bdarcus There is one thing I am not 100% happy with - the behavior of RET vs TAB. Maybe RET should only submit the current candidate if no candidates are selected. If candidates are selected only these candidates should be submitted and the current candidate should simply be ignored. This brings us back to the consult-completing-read-multiple discussion. By using only RET there everything is as intuitive as it gets at the cost of the double-RET.

@bdarcus
Copy link

bdarcus commented Jul 6, 2021

@minad

  1. OK
  2. yes, I know; maybe there should be a defcustom to swap that? I don't feel strongly about it though; you can wait to see what feedback you get. Also, as I said, I was finding it inconsistent; sometimes the string is preserved with TAB, and sometimes not.
  3. I think you misunderstand; if you look at that screenshot, the selected candidate is not a valid candidate; it's the initial-input value
  4. OK, so once embark adds that, it will work as it currently does under CRM? Edit: I don't understand the details of how Embark works here internally, but it does correctly pass a list to the commands with CRM, the way I am using it.

There is one thing I am not 100% happy with - the behavior of RET vs TAB. Maybe RET should only submit the current candidate if no candidates are selected. If candidates are selected only these candidates should be submitted and the current candidate should simply be ignored. This brings us back to the consult-completing-read-multiple discussion. By using only RET there everything is as intuitive as it gets at the cost of the double-RET.

I'm not totally following you on this. Maybe you can rephrase?

The double-RET issue was me just thinking selecting a single candidate may be the common case, so if someone is doing that a lot, they will get annoyed by it, and maybe an abused pinky finger.

@minad
Copy link
Owner Author

minad commented Jul 6, 2021

  1. No need for an additional defcustom. Just modify the keymap, it is a simple remapping.
  2. No, I understood this correctly. It is just that the initial-input filtering you are doing in bibtex-actions is not (or not yet) supported here. I hope we can figure out a better way on how to do this filtering and how initial-input should be interpreted by vertico-crm. The current interpretation interprets the initial-input string as candidates.
  3. Yes.

I'm not totally following you on this. Maybe you can rephrase?

I modified the RET behavior. If you select candidates with TAB and press RET, the current candidate position does not matter. RET only returns the current candidate when no candidates have been selected with TAB. In order to select three candidates you press TAB-TAB-TAB-RET. Before you had to press TAB-TAB-RET, which is one keypress less but also feels more confusing. If you select only a single candidate you can press RET only. Does this make sense?

@bdarcus
Copy link

bdarcus commented Jul 6, 2021

Does this make sense?

Yes, and it works well in my view. So 👍.

@bdarcus
Copy link

bdarcus commented Jul 6, 2021

This is looking really good @minad.

Are you convinced yet?

BTW, doing (setq vertico-count 20) makes my group sticking point a non-issue. I think doom will use 17, which should also work well.

@minad
Copy link
Owner Author

minad commented Jul 6, 2021

Actually there are complications. In order to implement this correctly, completion boundaries must be taken into account. E.g., for file completion the currently completed candidate string (the file name) is only part of the full candidate string (the path).

More detailed example: When completing a file path "~/.emacs.d/ini", the candidates are the files living in "~/.emacs.d/", e.g., "init.el". Selecting this file will add the path "~/.emacs.d/init.el" to the list of selected candidates. Then we would see the following UI, where we are completing file names and file paths at the same time. This is obviously incorrect.

Prompt: ~/.emacs.d/...
---- Selected ----
~/.emacs.d/init.el          <--- candidate is a file path
---- Not selected ----                                    
init.el                     <--- candidate is a file name
...

Therefore the currently implemented UI is insufficient in the general scenario. Of course this does not matter for your use case, where your candidates are always full candidates.

To implement the general case correctly, one probably ends up with an interface similar to (4) from our previous discussion. This means that all the logic must be baked into the UI directly (the vertico/vertico-crm split I tried here won't work out).

At this point every completion (even completing-read) can be generalized to additionally support marking multiple candidates which would potentially allow Embark multi actions (Embark does not yet support acting on multiple candidates as you are aware).

completing-read-multiple is only a trivial special case of the multi selection scenario, where the selected candidates are returned. So by implementing the feature correctly one almost necessarily ends up with the complicated full thing.

There are the two options:

  • Implement a restricted crm for the special case of full candidates (simple-crm). I should not pretend to have a full featured implementation here. simple-crm should rather live in your bibtex package directly as you have it in your bibtex pr. Then Vertico would not get an advanced crm implementation. The current primitive CRM solution with the comma separated candidates would still work though.
  • I implement the full featured solution. But this is only worth it as long given multi action support in Embark, since doing this only for completing-read-multiple is a lot of effort without most of the benefits.

However I just figured out that the default CRM implementation is also not fully compliant. Selecting multiple files separated by comma does not work and throws an error! This means that CRM by definition only allows tables without boundaries.

(completing-read-multiple "Multiple files: " #'read-file-name-internal)

So we are probably fine with the vertico-crm implementation as I have it. It is still worth to consider these edge case scenarios. In particular as soon as we have Embark actions there could be the desire to support multi selections during file completion in find-file. Then all these complications described above become relevant again.

@bdarcus
Copy link

bdarcus commented Jul 6, 2021

Interesting.

It sounds like, regardless of the ultimate path you choose, it will depend on enhancements to Embark already discussed a bit with @oantolin?

emacs-citar/citar#163 (comment)

E.g. even the "simple" path requires some Embark enhancements. Would your more full-featured option require different, or more extensive, enhancements?

@minad
Copy link
Owner Author

minad commented Jul 6, 2021

No, crm does not require embark enhancements. I meant that if one wants to implement the multi selection feature correctly, one almost automatically ends up with a full featured UI, where multi selections are fundamentally baked in. Multi selections are valuable for two things: 1. crm, 2. embark actions on multiple candidates. Implementing this only for crm does not seem worth it, for Embark more so. The question is where to stop - as soon as one implements the correct thing (and not only a few special cases), one ends up with the full featured UI. Since all of this is such a significant deviation from the status quo which doesn't handle multi selections at all, it may be better to stop right away. Alternatively implement only these special cases (simple-crm, vertico-crm, consult-completing-read-multiple), which ultimately break down. I wonder - in bibtex-actions is completing-read-multiple really needed? How does ivy-bibtex handle this given that ivy also doesn't have fancy multi selection support.

@bdarcus
Copy link

bdarcus commented Jul 6, 2021

No, crm does not require embark enhancements.

I thought you said it did?

To be clear, if I use CRM, and select three candidates, and then do embark-act and then bibtex-actions-insert-key, I get back three keys.

If I do the same with vertico-crm, I get one.

So something's missing somewhere. I was just assuming from previous discussions it was in Embark.

I meant that if one wants to implement the multi selection feature correctly, one almost automatically ends up with a full featured UI, where multi selections are fundamentally baked in. Multi selections are valuable for two things: 1. crm, 2. embark actions on multiple candidates. Implementing this only for crm does not seem worth it, for Embark more so.

Yes.

The question is where to stop - as soon as one implements the correct thing (and not only a few special cases), one ends up with the full featured UI. Since all of this is such a significant deviation from the status quo which doesn't handle multi selections at all, it may be better to stop right away. Alternatively implement only these special cases (simple-crm, vertico-crm, consult-completing-read-multiple), which ultimately break down.

Or do the latter in the short-term, and see if requirements and demand for the former emerges in time, along with implementation ideas. But that has other problems, I know.

I wonder - in bibtex-actions is completing-read-multiple really needed? How does ivy-bibtex handle this given that ivy also doesn't have fancy multi selection support.

Ivy-bibtex goes without feature-parity with the first package, which was helm-bibtex.

All of the functions derived from helm-bibtex into bibtex-completion take a list of keys as argument, but ivy-bibtex only ever submits a single key.

I think multi-selection make sense in a lot of cases here; creating a bibtex file out of filtered list, opening a list of notes or documents, inserting multi-cite citations (per below), etc.

If you don't have multi-selection, you have to do them each in turn, of course.

But not all of that requires CRM per se conceptually, in the sense a function that needs to operate on a list, rather than works on a list of items in sequence. Using embark-collect, for example, one can quickly run commands against a pre-selected subset, which can make that more efficient using CR.

An obvious exception is inserting a citation, where the result might be:

[cite:@key1;@key2;@key3]

With CR only, one would instead insert three separate keys, invoking that command three separate times. If you're inserting dozens of citation and their keys in a document, that can be hassle.

Or, per above, use embark-collect. But that has it's own problems, as while you can quickly insert keys in succession, you don't get the delimiters and such.

There is also the capf option.

So I guess the long-winded answer to your question of whether CRM is really needed is ... maybe.

@minad
Copy link
Owner Author

minad commented Jul 7, 2021

Regarding Embark - yes of course it needs support to support actions on multiple candidates. I meant that for CRM only (without actions) nothing is needed from the side of Embark. And even actions on single candidates work directly with Embark without special support.

@minad
Copy link
Owner Author

minad commented Jul 7, 2021

I have another idea regarding how CRM could be handled in general - going a route via Embark. I would like a solution which somehow fits into the whole picture. The idea is to add some ability to select candidates one by one, adding them to an *Embark Select/Collect* buffer. Maybe this idea is generally useful for Embark (e.g. collecting URLs across buffers).

  1. Add an embark-select action which can be executed anywhere and can be used to collect items. The items go to an *Embark Select/Collect* buffer
  2. Add embark-completing-read-multiple
  3. Embark collect buffers should support candidates of different types
  4. Embark collect buffers need a function to execute an action on all candidates
(defun embark-select ()
  (interactive)
  ;; Here the candidates should actually go to an embark-collect-mode buffer
  ;; Unfortunately embark-collect-mode buffers only support types of a single candidate (generalize this?)
  (let ((cand (cadr (embark--target)))
        (window-min-height 1)
        (window-resize-pixelwise t))
    (with-current-buffer (get-buffer-create "*Embark Select*")
      (goto-char (point-max))
      (unless (= (point) (point-min))
        (insert "\n"))
      (insert cand))
    (fit-window-to-buffer
     (display-buffer "*Embark Select*"
                     `(display-buffer-in-side-window
                       (window-parameters (mode-line-format . none))
                       (window-height . 1)
                       (side . bottom)
                       (slot . -1))))))

(defun embark-select-setup ()
  (lambda ()
    (with-current-buffer (get-buffer-create "*Embark Select*")
      (erase-buffer))))

(defun embark-completing-read-multiple (&rest args)
  (let ((item (apply #'completing-read args)))
    (or
     ;; If items have been selected with Embark, return them
     (mapcar #'substring-no-properties
             (split-string
              (with-current-buffer (get-buffer-create "*Embark Select*")
                (buffer-string))
              "\n" 'omit-nulls))
     ;; Otherwise return the single item
     (list item))))

(add-hook #'minibuffer-setup-hook #'embark-select-setup)
(define-key vertico-map [backtab] #'embark-select)

@bdarcus
Copy link

bdarcus commented Jul 7, 2021

I would like a solution which somehow fits into the whole picture.

As in, not specific to Vertico?

Or are you thinking vertico-crm and this could be complementary?

In any case, it seems an idea worth considering. The test would just be if the UI could be as elegant as this.

But I guess what you're thinking is this could be a way to act on multiple candidates in Embark, even without CRM?

@minad
Copy link
Owner Author

minad commented Jul 7, 2021

Since we don't have to concept of multi selections anywhere in our package set and it matters most for Embark actions, yes, I think it is worth to consider implementing the multi action/selection support in Embark. What I mean is to put Embark first and then look how the design affects the rest.

Implementing a general CRM in Vertico is a significant effort and complication as I described above and I don't feel comfortable adding a half baked solution here. I would be more comfortable with minad/consult#352 in a simplified form, which explicitly supports only a subset of scenarios (static completion tables, no completion boundaries). The function consult-completing-read-multiple would explicitly be a non-general solution. It would suffice for your use cases though. However given that it required an embark-consult addition and given that you didn't seem to be satisfied due to double-RET etc shows that it may be too weak. If one starts combining consult-completing-read-multiple with vertico-specific keybindings on the level of the user configuration one ends up with a very brittle solution overall.

  1. Add an explicitly weak solution to Consult if we deem it to be a significant improvement over the status quo (consult-completing-read-multiple consult#352). This would be the cheapest, least invasive addition.
  2. Add the richer vertico-crm solution to Vertico. However the solution is not fully correct regarding completion table support (this PR).
  3. Implement fully correct CRM support in Vertico (intrusive and complex). A lot of effort without benefits as long as Embark does not support acting on multiple candidates.
  4. Scrap the current CRM ideas. Implement general multi selections as actions on the level of Embark. It is probably not easy to end up with an elegant UI (my comment Add vertico-crm-mode in separate package #74 (comment)).

So I guess I would be happy with either 1. or 4. here. Of course 4. would be more satisfactory if one finds a good design. 1. is just a poor man solution, a stop gap measure.

Do you see my point? Does this make sense?

@bdarcus
Copy link

bdarcus commented Jul 7, 2021

Yes.

So we need to see what @oantolin thinks then in any case.

@minad
Copy link
Owner Author

minad commented Jul 7, 2021

Yes, it would be great to hear @oantolin's opinion about acting/collecting multiple candidates, in particular the idea in #74 (comment).

Independently we could still add minad/consult#352 to Consult. It would not hurt much and it may improve the status quo. But maybe you are spoiled now by the vertico-crm prototype in this PR? ;)

@bdarcus
Copy link

bdarcus commented Jul 7, 2021

Independently we could still add minad/consult#352 to Consult. It would not hurt much and it may improve the status quo. But maybe you are spoiled now by the vertico-crm prototype in this PR?

Probably yes to both; it is an improvement over CRM, but this is of course better.

EDIT: I forked your repo, so will maybe still personally use your branch code, even if you don't publish this package. In that scenario, I would still strongly recommend the Consult version on my README.

@bdarcus
Copy link

bdarcus commented Jul 7, 2021

BTW, I think the Embark angle is related to this question of mine and subsequent discussion from awhile back.

oantolin/embark#166

See in particular:

oantolin/embark#166 (comment)

I think I was asking that question when pondering whether to use CR or CRM.

I decided CRM made more sense, even if the UIs currently weren't ideal for it. It was, in effect, a bet on the future.

@oantolin
Copy link

oantolin commented Jul 7, 2021

This is very interesting! As you have probably guessed, I'm kind of swamped at work these days and haven't had that much time to work on Embark recently, but let me at least quickly write some disconnected thought about the Embark multiple-selection idea.

  • Generally speaking, I think it makes sense.
  • It might be tricky to make the UI comfortable for both these cases: (1) collecting multiple candidates from a single minibuffer, (2) collecting candidates from various different buffers and minibuffers. We can focus on (1) at first.
  • One should be wary of the distinction between mapcar and funcall: you might want to run a single-argument action on each candidate, or pass all candidates in a list to an action, or you may want the flexibility to do both. For example, bibtex-actions seems to require the latter because you probably want [cite:@key1;@key2] rather than [cite:@key1], [cite:@key2]. (Of course the single-argument action instead of "insert a new [cite:@key]" could be "if point is after an existing [cite:@key1;...] add the key at the end, otherwise start a new [cite:@key]").
  • Embark currently handles having candidates of disparate types in a single collect buffer through transformers, like for consult-buffer. This may be sufficient for the functionality desired here. If not, and more direct support for multiple types is added to Embark it would be nice to rework the consult-buffer support to use it.

@bdarcus
Copy link

bdarcus commented Jul 7, 2021

Thanks for weighing in @oantolin!

  • you might want to run a single-argument action on each candidate, or pass all candidates in a list to an action, or you may want the flexibility to do both.

I was thinking the latter, if it's possible.

The first allows support for the vastly wider array of CR commands, while the second those, like mine, which rely on CRM.

I looked again at my list of commands. I think the only two that really should must operate on a list are:

  • bibtex-actions-insert-citation
  • bibtex-actions-insert-key

... since in both cases, you're basically running mapconcat, concat, etc. on that list of keys.

The others can just as well run the same action on the different candidates in sequence.

@minad minad closed this Jul 7, 2021
@minad
Copy link
Owner Author

minad commented Jul 7, 2021

I closed this in favor of the consult crm implementation, which is pretty much equivalent to the vertico-crm version, only UI agnostic. The consult version does not yet offer better keybindings (TAB/RET) as we discussed, but this is easy to add in the consult-vertico/selectrum integration code.

Independent of that, I would love to see Embark support for actions on multiple candidates at once (calling an action in a loop for each candidate). As described before this should ideally even allow selecting multiple candidates in plain completing-read completions, e.g., find-file.

I think Embark multi actions/selections are orthogonal to the CRM discussion, therefore I went forward with consult-completing-read-multiple. If it turns out that we find a much better all-encompassing solution based on Embark, this can easily be reverted.

@bdarcus
Copy link

bdarcus commented Jul 8, 2021

I was just looking at this new (in the past day or so) code for org-cite-insert (to be included in org-mode).

https://code.orgmode.org/bzg/org-mode/src/wip-cite-new/lisp/oc-basic.el#L703

In his first draft, he used CRM. Now he's using CR.

In that UI, you add the keys to the prompt, and then exit to complete the list and insert the citation.

Just curious: WDYT of that alternative approach?

@minad
Copy link
Owner Author

minad commented Jul 8, 2021

This is much simpler but the downside is that you cannot remove keys anymore it seems. And you also need the double-RET. It seems to be pretty common in Org that they are reinventing the wheel somehow all over the place. It would be better if they just use completing-read-multiple. Then Org would also profit from enhancements from other packages, like consult-completing-read-multiple.

@bdarcus
Copy link

bdarcus commented Jul 8, 2021

To be fair, he started with CRM and completing the keys.

When I and others pointed he needed to complete on the data and return the key (which necessarily means longer candidates), he went to this solution, almost surely because he ran into the same UI issues with CRM as I did.

And yes, you can't edit the keys.

@minad
Copy link
Owner Author

minad commented Jul 8, 2021

What is the relation between the new org functionality and your bibtex-actions package btw?

@bdarcus
Copy link

bdarcus commented Jul 8, 2021

I'll just be able to plug in some of these functions into the standardized org infrastructure.

So bibtex-actions-at-point (including embark-act) will be accessible from org-open-at-point when on a citation, and bibtex-actions-insert-citation will be accessible from org-cite-insert.

The new functionality is nicely modular.

So out-of-box the user will have some baseline functionality, but if they do this, they get bibtex-actions instead.

(setq org-cite-insert-processor 'bibtex-actions)
(setq org-cite-follow-processor 'bibtex-actions)

That infrastructure also provides better contextual functionality. The insert functionality will behave differently depending on what part of the citation point is on, for example, without me having to write that.

I started this project in part because I wanted to target this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants